feat(platform): implement graph-level Safe Mode toggle for HITL blocks #11455
Conversation
github-project-automation
bot
moved this to 🆕 Needs initial review
in AutoGPT development kanban
github-actions
bot
added
platform/frontend
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you for this comprehensive PR implementing a graph-level Safe Mode toggle for HITL blocks. The code changes look solid, and your PR description does an excellent job of explaining the intent and implementation details. However, I noticed that you haven't checked off any of the items in your test plan. Before this can be merged, please:
This feature affects execution behavior for HITL blocks, so thorough testing is particularly important to ensure it works as expected across different contexts (builder and library pages). Also, I noted a few of your known issues:
While these don't necessarily block merging, it would be good to know if you have plans to address them in this PR or follow-up PRs. Let me know if you need any clarification or have questions about these requirements. |
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Thanks for the comprehensive PR implementing the Safe Mode toggle for HITL blocks! The implementation looks thorough and well-thought-out. I have a few comments before this is ready to merge: Test Plan CompletionThe test plan is clearly specified in your PR description, but none of the items are checked off. Could you please complete the testing as outlined and check off the test plan items? Specifically:
Known IssuesYou've done a great job documenting the known issues. Since these are marked as "Work in Progress", could you clarify which of these need to be resolved before merging versus which can be addressed in follow-up PRs? Particularly concerning are these high priority issues:
Technical Questions
Migration ReadinessThe migration looks good, but let's ensure it's been tested on a representative database. Have you verified the migration runs successfully on a database with existing graphs? Overall, this is a well-implemented feature with good cross-cutting concerns addressed. Looking forward to getting the test plan completed! |
majdyz
force-pushed
the
feat/human-in-the-loop-block-toggle
branch
from
121ceb6 to
4e686d7
Compare
|
Thank you for this detailed PR implementing the graph-level Safe Mode toggle for HITL blocks! FeedbackYour implementation looks comprehensive, covering both backend and frontend changes needed for this feature. I appreciate the detailed description with sections for backend changes, frontend changes, technical implementation, and known issues. A few observations:
Once you've added the standard PR checklist with your test plan and addressed the known issues (or planned follow-ups), this should be ready for review again. |
|
Thank you for this PR implementing a graph-level Safe Mode toggle for HITL blocks. The feature looks well-designed and adds important safety functionality to the platform. However, there's one key issue that needs to be addressed before this PR can be merged: Required Changes
Additional Feedback
Once you add the required checklist, this PR should be ready for review and potential approval. |
github-actions
bot
added
the
conflicts
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
majdyz
force-pushed
the
feat/human-in-the-loop-block-toggle
branch
from
1a15c4a to
a8e4de3
Compare
✅ Deploy Preview for auto-gpt-docs-dev canceled.
|
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
github-actions
bot
removed
the
conflicts
✅ Deploy Preview for auto-gpt-docs canceled.
|
|
Thank you for submitting this comprehensive PR implementing the graph-level Safe Mode toggle for HITL blocks! The changes look well-structured with both backend and frontend components properly addressed. However, before we can merge this PR, please address the following issues: Missing Checklist
Technical Feedback
Once you've addressed these points, particularly completing the checklist, this PR should be ready for review and merging. The implementation itself looks solid, with proper database migrations, user_id handling, and appropriate type definitions. |
majdyz
force-pushed
the
feat/human-in-the-loop-block-toggle
branch
from
44a16c1 to
98c1377
Compare
github-actions
bot
added
the
conflicts
|
This pull request has conflicts with the base branch, please resolve those so we can evaluate the pull request. |
majdyz
force-pushed
the
feat/human-in-the-loop-block-toggle
branch
from
b555336 to
2d4595f
Compare
…at/human-in-the-loop-block-toggle
|
Conflicts have been resolved! 🎉 A maintainer will review the pull request shortly. |
github-actions
bot
removed
the
conflicts
majdyz
requested review from
Swiftyos and
kcze
and removed request for
a team
|
Thank you for this comprehensive PR implementing the graph-level Safe Mode toggle for HITL blocks. The changes look well-structured with a clear separation between backend and frontend components. Before this PR can be merged, please add the required checklist to your PR description. Since this is a substantial code change (labeled as size/xl), we need the checklist to ensure all necessary testing and verification has been completed. A few additional suggestions:
The implementation itself looks solid - I particularly like how you've:
Once you've added the checklist and addressed any merge conflicts, this PR should be ready for another review. |
|
Thanks for this comprehensive PR implementing the graph-level Safe Mode toggle for HITL blocks. The feature seems well-designed, covering both backend and frontend aspects. Good Implementation Points
Areas Needing Attention
Once you've completed the test plan and addressed the high-priority issues, this PR will be ready for another review. The core implementation looks solid! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return GraphSettings.model_validate(lib.settings) | ||
| except Exception: | ||
| logger.warning( | ||
| f"Malformed settings for LibraryAgent user={user_id} graph={graph_id}" |
Check failure
Code scanning / CodeQL
Clear-text logging of sensitive information High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 hour ago
The best fix is to remove or redact sensitive user-specific information from log messages. In the specific case of autogpt_platform/backend/backend/data/graph.py, at line 1138, the warning logs both user_id and graph_id in cleartext. Rather than logging these values directly, we should instead use a generic message, e.g. "Malformed settings for LibraryAgent" without referencing specific IDs, or redact them (e.g. logging only a shortened prefix, or hashing them, or using a placeholder).
Recommended fix for the affected region:
- Change the logger.warning call at line 1138 so that
user_idandgraph_idare not logged in their entirety. - If operational value is needed, log only that a problem occurred, omitting IDs; if necessary for debugging, include only partial/hashing, or instruct to query with context outside logs.
- This is a one-line change inside the function
get_graph_settingsin autogpt_platform/backend/backend/data/graph.py.
No additional imports or functions/methods are needed.
| @@ -1135,7 +1135,7 @@ | ||
| return GraphSettings.model_validate(lib.settings) | ||
| except Exception: | ||
| logger.warning( | ||
| f"Malformed settings for LibraryAgent user={user_id} graph={graph_id}" | ||
| "Malformed settings for LibraryAgent (redacted identifiers)" | ||
| ) | ||
| return GraphSettings() | ||
|
|
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| if not user: | ||
| raise HTTPException(status_code=404, detail="User not found.") | ||
|
|
||
| user_context = UserContext(timezone=user.timezone) | ||
|
|
||
| start_time = time.time() | ||
| try: | ||
| output = defaultdict(list) | ||
| async for name, data in obj.execute( | ||
| data, | ||
| user_context=user_context, | ||
| user_id=user_id, | ||
| # Note: graph_exec_id and graph_id are not available for direct block execution | ||
| ): |
There was a problem hiding this comment.
Bug: Missing execution_context in execute_graph_block() for HITL blocks.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The execute_graph_block() endpoint in v1.py calls obj.execute() without providing the execution_context parameter. The HumanInTheLoop block's run() method, which is eventually called, requires execution_context as a keyword-only argument. This omission will cause a TypeError: missing 1 required positional argument: 'execution_context' when a HITL block is executed directly via this endpoint. This is a regression as user_context (the predecessor to execution_context) was previously passed.
💡 Suggested Fix
Ensure execution_context is properly initialized and passed to obj.execute() within the execute_graph_block endpoint, similar to how user_context was handled previously.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: autogpt_platform/backend/backend/server/routers/v1.py#L387-L401
Potential issue: The `execute_graph_block()` endpoint in `v1.py` calls `obj.execute()`
without providing the `execution_context` parameter. The `HumanInTheLoop` block's
`run()` method, which is eventually called, requires `execution_context` as a
keyword-only argument. This omission will cause a `TypeError: missing 1 required
positional argument: 'execution_context'` when a `HITL` block is executed directly via
this endpoint. This is a regression as `user_context` (the predecessor to
`execution_context`) was previously passed.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4284451
| ) | ||
| if parent_exec and parent_exec.status == ExecutionStatus.TERMINATED: | ||
| logger.info( | ||
| f"[{self.service_name}] Skipping execution {graph_exec_id} - parent {parent_graph_exec_id} is TERMINATED" | ||
| ) | ||
| # Mark this child as terminated since parent was stopped | ||
| get_db_client().update_graph_execution_stats( | ||
| graph_exec_id=graph_exec_id, | ||
| status=ExecutionStatus.TERMINATED, | ||
| ) | ||
| _ack_message(reject=False, requeue=False) | ||
| return | ||
| except Exception as e: | ||
| logger.warning( | ||
| f"[{self.service_name}] Could not check parent status for {graph_exec_id}: {e}" | ||
| # Mark this child as terminated since parent was stopped | ||
| get_db_client().update_graph_execution_stats( | ||
| graph_exec_id=graph_exec_id, | ||
| status=ExecutionStatus.TERMINATED, | ||
| ) | ||
| # Continue execution if parent check fails (don't block on errors) | ||
| _ack_message(reject=False, requeue=False) | ||
| return | ||
|
|
||
| # Check user rate limit before processing | ||
| try: |
There was a problem hiding this comment.
Bug: Unhandled database exception in _handle_run_message.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The _handle_run_message function in manager.py removes a try-except block around the get_db_client().get_graph_execution_meta() call. If this database operation fails (e.g., due to a database connection error or timeout), the unhandled exception will propagate, causing the message handler to crash and potentially terminating the entire consumer thread. The _handle_run_message method is not decorated with @error_logged, so exceptions are not gracefully handled.
💡 Suggested Fix
Re-introduce robust error handling, such as a try-except block, around the get_db_client().get_graph_execution_meta() call within _handle_run_message to prevent crashes from database failures.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: autogpt_platform/backend/backend/executor/manager.py#L1556-L1587
Potential issue: The `_handle_run_message` function in `manager.py` removes a
`try-except` block around the `get_db_client().get_graph_execution_meta()` call. If this
database operation fails (e.g., due to a database connection error or timeout), the
unhandled exception will propagate, causing the message handler to crash and potentially
terminating the entire consumer thread. The `_handle_run_message` method is not
decorated with `@error_logged`, so exceptions are not gracefully handled.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 4284451
|
Thank you for this comprehensive PR implementing the graph-level Safe Mode toggle for HITL blocks. The implementation looks solid overall! Missing Requirements
ImplementationThe technical implementation looks strong with good separation of concerns:
Good job addressing the needs across backend, frontend and blocks! Known IssuesYou've done a good job documenting the known issues. Once the checklist is completed, make sure your test plan verifies that these issues don't critically impact functionality. Next Steps
Once you've addressed these points, the PR should be ready for review again. |
|
Thank you for this comprehensive PR! The implementation of a graph-level Safe Mode toggle for HITL blocks looks well-structured and follows good practices. A few observations and suggestions:
Overall this is a well-structured feature implementation. Please address the unchecked test plan items and high priority known issues before merging. |
|
Thanks for this detailed PR implementing the graph-level Safe Mode toggle for HITL blocks. The implementation looks solid across backend, frontend, and blocks components. Missing ChecklistYou've provided a thorough description of the changes and even included a test plan section, but the PR is missing the required checklist from the template. Please add the standard checklist section and check off the relevant items. Code Review Comments
Please update your PR to include the required checklist, and consider addressing the high-priority known issues you've identified before final approval. |
Summary
This PR implements a graph-level Safe Mode toggle system for Human-in-the-Loop (HITL) blocks. When Safe Mode is ON (default), HITL blocks require manual review before proceeding. When OFF, they execute automatically.
🔧 Backend Changes
metadataJSON column toAgentGraphtable with migrationexecute_graphendpoint to acceptsafe_modeparameterhas_human_in_the_loopfor graphs containing HITL blocks8b2a7b3c-6e9d-4a5f-8c1b-2e3f4a5b6c7d🎨 Frontend Changes
FloatingSafeModeTogglewith dual variants:usePutV1UpdateGraphVersion🛠 Technical Implementation
safe_modeparameter takes precedence over graph metadatahas_human_in_the_loopmetadata field🚧 Known Issues (Work in Progress)
High Priority
Medium Priority
Low Priority
safe_modeparameter to execution calls once OpenAPI is regenerated🧪 Test Plan
🔗 Related
🤖 Generated with Claude Code